gh-130824: Add tests for `NULL` in `PyLong_*AndOverflow` functions by ZeroIntensity · Pull Request #130828 · python/cpython

ZeroIntensity · 2025-03-04T11:30:36Z

It crashed before because the assertion didn't realize that overflow would be 0 and not -1 in SystemError cases.

Issue: PyLong_AsLongAndOverflow does not guarantee overflow is -1 when value is -1 #130824

serhiy-storchaka

LGTM. 👍

skirpichev

LGTM

Few nitpicks, feel free to ignore.

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

serhiy-storchaka · 2025-03-04T15:44:15Z

        self.assertEqual(func(max_val + 1), (-1, +1))
+        self.assertRaises(SystemError, func, None)

        # CRASHES func(1.0)


Does it really crash?

Good catch, it doesn't: "If any other exception occurs set *overflow to 0 and return -1 as usual." TypeError will be here.

serhiy-storchaka · 2025-03-04T15:48:26Z

    if (value == -1 && PyErr_Occurred()) {
-        assert(overflow == -1);
+        // overflow can be 0 if a seperate exception occurred
+        assert(overflow == -1 || overflow == 0);


Is not it always 0?

Not with OverflowError.

Yeah, OverflowError is -1 and then other exceptions (only SystemError?) are 0.

only SystemError?

No, TypeError too.

OverflowError is not raised. In case of an integer overflow, *overflow should be 1 or -1.

Isn't it always 0 here?

skirpichev

one grammar nitpick

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

miss-islington-app · 2025-03-05T08:59:06Z

Thanks @ZeroIntensity for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington-app · 2025-03-05T08:59:06Z

Thanks @ZeroIntensity for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington-app · 2025-03-05T08:59:09Z

Sorry, @ZeroIntensity and @encukou, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 90130807d9c5a55b2a64024f5dfbee4785b9a27c 3.12

…ons (pythonGH-130828) (cherry picked from commit 9013080) Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

bedevere-app · 2025-03-05T08:59:18Z

GH-130869 is a backport of this pull request to the 3.13 branch.

ZeroIntensity · 2025-03-05T10:37:12Z

@encukou Should I make the manual backport to 3.12? I'm not sure it's worth backporting to prevent conflicts if there's already a conflict.

serhiy-storchaka · 2025-03-05T10:41:49Z

It will help to backport future tests. It will help to ensure that no regressions be introduced in old versions.

See also #130871. You can include that change in the backports.

… functions (pythonGH-130828) (cherry picked from commit 9013080) Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

bedevere-app · 2025-03-05T10:55:19Z

GH-130876 is a backport of this pull request to the 3.12 branch.

encukou · 2025-03-05T10:55:49Z

Should I make the manual backport to 3.12?

Sorry, I did it before I saw your message

… functions (pythonGH-130828) (cherry picked from commit 9013080) Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

…ions (GH-130828) (GH-130869) (cherry picked from commit 9013080) Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

…GH-130828) (GH-130876) (cherry picked from commit 9013080) Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

pythongh-130824: Add tests for NULL in PyLong_*AndOverflow functions

b15c219

ZeroIntensity added tests Tests in the Lib/test dir skip news labels Mar 4, 2025

ZeroIntensity requested a review from serhiy-storchaka March 4, 2025 11:30

bedevere-app Bot added the awaiting review label Mar 4, 2025

bedevere-app Bot mentioned this pull request Mar 4, 2025

PyLong_AsLongAndOverflow does not guarantee overflow is -1 when value is -1 #130824

Closed

serhiy-storchaka approved these changes Mar 4, 2025

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting review labels Mar 4, 2025

skirpichev approved these changes Mar 4, 2025

View reviewed changes

Comment thread Lib/test/test_capi/test_long.py Outdated

Comment thread Modules/_testlimitedcapi/long.c

Apply suggestions from code review

0fc0c21

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

serhiy-storchaka reviewed Mar 4, 2025

View reviewed changes

serhiy-storchaka self-requested a review March 4, 2025 15:50

skirpichev self-requested a review March 5, 2025 00:26

Add check for TypeError

46cd27f

skirpichev approved these changes Mar 5, 2025

View reviewed changes

Comment thread Modules/_testlimitedcapi/long.c Outdated

Comment thread Modules/_testlimitedcapi/long.c Outdated

Apply suggestions from code review

8bb2502

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

encukou merged commit 9013080 into python:main Mar 5, 2025

bedevere-app Bot removed the awaiting merge label Mar 5, 2025

encukou added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Mar 5, 2025

miss-islington-app Bot assigned encukou Mar 5, 2025

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Mar 5, 2025

ZeroIntensity deleted the add-pylong-null-checks branch March 5, 2025 10:24

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Mar 5, 2025

serhiy-storchaka mentioned this pull request Apr 5, 2025

gh-130824: Clean up test wrappers for PyLong_*AndOverflow functions #130871

Merged

Uh oh!

Uh oh!

Conversation

ZeroIntensity commented Mar 4, 2025 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

skirpichev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

skirpichev Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

skirpichev Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

skirpichev Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

skirpichev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miss-islington-app Bot commented Mar 5, 2025

Uh oh!

miss-islington-app Bot commented Mar 5, 2025

Uh oh!

miss-islington-app Bot commented Mar 5, 2025

Uh oh!

bedevere-app Bot commented Mar 5, 2025

Uh oh!

ZeroIntensity commented Mar 5, 2025

Uh oh!

serhiy-storchaka commented Mar 5, 2025

Uh oh!

bedevere-app Bot commented Mar 5, 2025

Uh oh!

encukou commented Mar 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ZeroIntensity commented Mar 4, 2025 •

edited by bedevere-app Bot

Loading